fix(mcp): prevent MCPClient from hanging on exit#1847
fix(mcp): prevent MCPClient from hanging on exit#1847giulio-leone wants to merge 1 commit intostrands-agents:mainfrom
Conversation
giulio-leone
requested a deployment
to
manual-approval
GitHub Actions
Waiting
giulio-leone
requested a deployment
to
manual-approval
GitHub Actions
Waiting
giulio-leone
force-pushed
the
fix/mcp-client-hang-on-exit-1732
branch
from
6c8bd31 to
9af45d9
Compare
giulio-leone
requested a deployment
to
manual-approval
GitHub Actions
Waiting
giulio-leone
requested a deployment
to
manual-approval
GitHub Actions
Waiting
|
Friendly ping — CI is green, tests pass, rebased on latest. Ready for review whenever convenient. Happy to address any feedback. 🙏 |
…ad is unresponsive MCPClient.stop() called _background_thread.join() without a timeout, causing the process to hang indefinitely when the background thread could not be signaled (e.g., event loop already closed during interpreter shutdown or finalizer invocation). Changes: - Add cleanup_timeout parameter (default 5.0s) to MCPClient.__init__ - Use timeout in _background_thread.join() to prevent indefinite hangs - Catch RuntimeError from run_coroutine_threadsafe when the event loop is already closed - Log a warning when the thread does not stop within the timeout Fixes strands-agents#1732
giulio-leone
force-pushed
the
fix/mcp-client-hang-on-exit-1732
branch
from
9af45d9 to
9351ba6
Compare
giulio-leone
requested a deployment
to
manual-approval
GitHub Actions
Waiting
giulio-leone
requested a deployment
to
manual-approval
GitHub Actions
Waiting
|
Friendly ping — rebased on latest and ready for review. Happy to address any feedback! |
|
/strands review |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Issue: Missing "Documentation PR" section in PR description. Suggestion: Please add a "Documentation PR" section to the PR description. Since this adds a new If you believe the new parameter warrants user documentation (e.g., for users experiencing slow MCP server shutdowns), please link a documentation PR. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Assessment: Request Changes
Well-implemented fix for the MCPClient hanging issue. The approach of adding a timeout to join() and gracefully handling closed event loops is correct and follows the existing MCP client architecture patterns.
Review Summary
- Code Quality: Clean implementation with proper type annotations, docstrings, and error handling
- Testing: Comprehensive tests covering the regression case and edge conditions
- Backward Compatibility: New
cleanup_timeoutparameter with sensible default preserves existing behavior - Documentation: PR description is missing a "Documentation PR" section - please add either a link to a docs PR or a justification for why documentation updates aren't needed
Once the Documentation PR section is added, this is ready to merge. 👍
|
The "Documentation PR" section has been added to the PR description ✅
Could the bot review be re-run or dismissed? Happy to address any other feedback. |
|
@mkmeral Could you re-run |
|
Hi — the |
|
It seems like we have merged in a fix last week for the given hanging issue. #1830 I will close this PR in favor of that one for now. That PR is a better approach as it fixes the underlying issue |
2 participants
Bug
MCPClient.stop()hangs indefinitely when the background thread cannot be stopped, causing scripts to freeze on exit.Reported in: #1732
Root cause
_background_thread.join()was called without a timeout. Whenstop()is triggered fromAgent.__del__()(garbage collector) or during interpreter shutdown, the event loop may already be closed, sorun_coroutine_threadsafe(_set_close_event(), loop)silently fails (raisesRuntimeError). The background thread never receives the shutdown signal, andjoin()blocks forever.Fix
cleanup_timeoutparameter (default 5.0 seconds) toMCPClient.__init__()join(timeout=cleanup_timeout)instead of indefinitejoin()RuntimeErrorfromrun_coroutine_threadsafewhen the event loop is already closedTesting
test_stop_does_not_hang_when_background_thread_unresponsive— verifies timeout prevents hangtest_stop_handles_closed_event_loop— verifies graceful handling of closed event loopjoin()assertions to verify timeout parameterFixes #1732
Documentation PR
Not required — this is an internal bug fix that adds a timeout parameter to
MCPClient.stop(). The existing API surface is unchanged; the newcleanup_timeoutparameter is optional with a sensible default.